Skip to content

[rust] fix Windows architecture detection under WOW64 - #17987

Merged
titusfortner merged 1 commit into
SeleniumHQ:trunkfrom
titusfortner:windows-arch-detection
Sep 6, 2026
Merged

[rust] fix Windows architecture detection under WOW64#17987
titusfortner merged 1 commit into
SeleniumHQ:trunkfrom
titusfortner:windows-arch-detection

Conversation

@titusfortner

Copy link
Copy Markdown
Member

🔗 Related Issues

#15363 transitioned Selenium Manager off of WMIC when Microsoft deprecated it. The PowerShell replacement was significantly less performant, so a different approach was taken by commit 9af3d6e to get the architecture value from an environment variable. Two problems with this, the first is that the variable chosen reported what the binary was built with (x86), not the architecture of the system, and the second is that the conditional used for checking the value continued to compare against WMIC values (e.g., contains("32")), which resulted in the default else statement always being used (ARCH_X64).

I think this is likely the underlying concern of #15801. Running Selenium Manager with emulation isn't nearly as big of a deal as not being able to get native browser/driver binaries.

💥 What does this PR do?

Fixes Windows architecture detection, which has resolved every Windows host as x86_64 since 4.30.0 (verified from Plausible data)

Since win64 binaries are always downloaded, 32-bit machines are completely broken since they can't run 64 bit binaries.
Windows ARM users can run x64 Selenium Manager with emulation, but the x64 versions of Firefox and Edge are likely ~1.5x less performant than the native ARM binaries will be.

🔧 Implementation Notes

  • Use GetNativeSystemInfo to get accurate architecture value and remove PROCESSOR_ARCHITECTURE which does not give useful information.
  • The regression survived this long because CI doesn't run on 32-bit and there's no easy way to add a test for this.

🤖 AI assistance

  • No substantial AI assistance used
  • AI assisted (complete below)
    • Tool(s): Claude Code (Claude Opus 5)
    • What was generated: diagnosis, fix, and this description
    • I reviewed all AI output and can explain the change

🔄 Types of changes

  • Bug fix (backwards compatible)

🤖 Generated with Claude Code

@selenium-ci selenium-ci added C-rust Rust code is mostly Selenium Manager B-manager Selenium Manager labels Sep 6, 2026
@qodo-code-review

Copy link
Copy Markdown
Contributor

PR Summary by Qodo

Fix Windows architecture detection under WOW64

🐞 Bug fix 🕐 10-20 Minutes

Grey Divider

AI Description

• Detect native Windows architecture through GetNativeSystemInfo, including WOW64 hosts.
• Map x86, ARM, and default 64-bit results to Selenium architecture identifiers.
• Remove environment-based detection that incorrectly forced x64 downloads.
Diagram

graph TD
    A["Manager config"] --> B{"Windows host?"} -->|Yes| C["Native system info"] --> D{"CPU architecture"}
    B -->|No| H["uname detection"]
    D -->|Intel| E["x86 target"]
    D -->|ARM or ARM64| F["ARM64 target"]
    D -->|Other| G["x64 target"]
Loading
High-Level Assessment

The direct GetNativeSystemInfo approach is appropriate because it reports the native host architecture even when Selenium Manager runs under WOW64. Environment variables report process architecture, while PowerShell adds unnecessary startup overhead; neither is preferable for this requirement.

Files changed (1) +15 / -26

Bug fix (1) +15 / -26
config.rsDetect the native Windows architecture through WinAPI +15/-26

Detect the native Windows architecture through WinAPI

• Replaces 'PROCESSOR_ARCHITECTURE' parsing with 'GetNativeSystemInfo', ensuring WOW64 processes detect the host architecture rather than the executable architecture. Maps Intel to x86, ARM variants to ARM64, and remaining architectures to x64, with a non-Windows compilation stub.

rust/src/config.rs

@qodo-code-review

qodo-code-review Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Grey Divider


Action required

1. Maintainers miss Windows regressions ✗ Dismissed 📘 Rule violation ☼ Reliability
Description
get_win_os_architecture maps native processor values to x86, ARM64, or x64 without any focused
test covering those branches. A future mapping change can therefore select incompatible browser and
driver binaries without being detected by the existing generic architecture tests.
Code

rust/src/config.rs[R366-369]

+            si if si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_INTEL => ARCH_X86,
+            si if si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_ARM
+                || si.wProcessorArchitecture == PROCESSOR_ARCHITECTURE_ARM64 =>
+            {
Evidence
PR Compliance ID 4 requires focused tests for changed behavior. The cited implementation introduces
multiple Windows architecture branches, while the PR changes no test file and repository search
found no test or other invocation of get_win_os_architecture.

AGENTS.md: Add Focused Tests for Implemented Behavior
rust/src/config.rs[359-375]
rust/tests/config_unit_tests.rs[73-100]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new Windows native processor-architecture mapping has no focused test coverage.

## Issue Context
Extract the processor-value conversion into a testable function and cover Intel, ARM, ARM64, AMD64, and unknown values without requiring each architecture in CI.

## Fix Focus Areas
- rust/src/config.rs[359-375]
- rust/tests/config_unit_tests.rs[73-100]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Context sources
✅ Web pages:
  +2 more
Review mode: ⚖️ Balanced: This is a behavior-changing Windows platform detection fix with significant compatibility and binary-selection impact, but the logic is localized enough for one careful review pass.

Grey Divider

Tip of the day
💡 Did you know, you can show, collapse, or hide each part of a finding: code, evidence, and all

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Grey Divider

Qodo Logo

Comment thread rust/src/config.rs
@titusfortner
titusfortner merged commit ed2dda5 into SeleniumHQ:trunk Sep 6, 2026
59 checks passed
This was referenced Sep 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

B-manager Selenium Manager C-rust Rust code is mostly Selenium Manager

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants